Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add geosearch autocomplete. #192

Merged
merged 26 commits into from
Sep 23, 2018
Merged

Add geosearch autocomplete. #192

merged 26 commits into from
Sep 23, 2018

Conversation

toolness
Copy link
Collaborator

@toolness toolness commented Sep 21, 2018

This progressively enhances onboarding step 1 to use the NYC Planning Labs autocomplete API, powered by the excellent Downshift React component.

If any network or runtime errors occur in the component, the app will fall back to the baseline experience (the separate address and borough fields) using a new <ProgressiveEnhancement> component that this PR introduces.

2018-09-21_9-12-58

To do

  • Add tests.
  • Add docs.
  • Consider showing a loading spinner while requests are being fetched (Bulma has a class for it).
  • Make sure it works on screen readers. Update: I tried it out on NVDA and Narrator and it actually works fairly well, although some announcements are repeated multiple times. But I'm very impressed with how well Downshift works out-of-the-box in this regard.
  • Only show the first few results (the API gives us 10, which is way too many, and I haven't figured out how to limit that to use less network bandwidth, but at least we can trim it on our end).
  • Refactor until CodeClimate is happy (at the very least).
  • The user shouldn't actually be able to submit the form until they've selected a valid address that has the street and borough information. We can punt on this, now that the field shows server errors and only displays a non-empty value if it's a valid one.
  • If the autocomplete ever loses focus, the entire text input disappears, which is unfortunate. We should either preserve the text input or auto-select the first suggestion or something. Update: the field always reverts to the last valid value, so I think this is good to go for now, though we'll see during user testing if folks are confused by the behavior.
  • If the server says that either the address or borough field has errors, which would be very unusual, we should probably downgrade to the baseline experience just to be safe. We deal with the "this field is required" case now, which is going to be the vast majority of use cases, so I'm not going to worry about the extremely unlikely edge case where the server thinks the address is invalid.
  • If the autocomplete component throws any errors or experiences network problems, we should probably downgrade too.

@toolness toolness added this to the The First Milestone milestone Sep 21, 2018
@toolness toolness changed the title [WIP] Add geosearch autocomplete. Add geosearch autocomplete. Sep 22, 2018
@@ -59,7 +64,7 @@ export default class ReactTestingLibraryPal {
const input = this.rr.getByLabelText(matcher, {
selector: 'input, select'
}) as HTMLInputElement;
input.value = value;
rt.fireEvent.change(input, { target: { value } });
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important because it causes the onChange handlers of the input fields to be called, if they are set, which is particularly important for the autocomplete widget. I actually had to get assistance from the Downshift community to figure this out and they were very quick to help out.

@@ -230,6 +230,11 @@
MIDDLEWARE.append(
'rollbar.contrib.django.middleware.RollbarNotifierMiddlewareExcluding404')

CSP_CONNECT_SRC = [
"'self'",
"https://geosearch.planninglabs.nyc"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this could get out-of-sync with the actual web service we use to perform the geo search but um, at least the progressive enhancement error boundary will kick in and it will downgrade to baseline. Also, it is extremely unlikely to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant